Conversation
apps/web/components/organisms/newOpinionForm/newOpinionForm.tsx
Outdated
Show resolved
Hide resolved
apps/web/components/organisms/protectedPage/protectedComponent/protectedComponent.tsx
Outdated
Show resolved
Hide resolved
purple review Co-authored-by: prplblck <55458485+goodideagiver@users.noreply.github.com>
apps/web/components/organisms/newOpinionForm/newOpinionForm.tsx
Outdated
Show resolved
Hide resolved
Eghizio
left a comment
Eghizio
left a comment
There was a problem hiding this comment.
Nice work. Left some comments out of curiosity and some of my personal thoughts 😉
This was my first experience with this project so excuse some of my questions 😄
Also, great job on the docs in the README about setting up the project. It helped a lot 💪
apps/web/components/molecules/commentItem/commentItemWithReply.tsx
Outdated
Show resolved
Hide resolved
| <div className="flex-1 px-2 ml-2 text-sm font-medium leading-loose text-gray-600"> | ||
| {comment} | ||
| </div> | ||
| <button className="inline-flex items-center px-1 pt-2 ml-1 flex-column"> |
There was a problem hiding this comment.
This 2 Reply and Like buttons share styles. Same with the div inside. I'd make a component out of this.
The only difference is the pt-2 which can be applied on some container or spacer.
apps/web/components/molecules/commentItem/commentItemWithReply.tsx
Outdated
Show resolved
Hide resolved
apps/web/components/molecules/commentItem/commentItemWithReply.tsx
Outdated
Show resolved
Hide resolved
apps/web/components/organisms/newOpinionForm/newOpinionForm.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| type ProtectedComponentProps = { | ||
| info?: string; | ||
| children: ReactNode; |
There was a problem hiding this comment.
Maybe adding an optional slot for a component that should be displayed in case user is not logged in?
| </div> | ||
| </section> | ||
| <section> | ||
| <Tabs /> |
There was a problem hiding this comment.
Maybe something like DiscussionTabs or make it some discussion section?
apps/web/types/types.tsx
Outdated
| author: string; | ||
| avatar: string; | ||
| comment: string; | ||
| date: number; |
There was a problem hiding this comment.
Is it UTC timestamp? Are you thinking about specific users from specific timezone? (target)
| </button> | ||
| <button className="inline-flex items-center px-1 -ml-1 flex-column"> | ||
| <div className="w-5 h-5"> | ||
| <Liked /> |
There was a problem hiding this comment.
This buttons with icons are a good candidate for a separate component as well ;)
| comment, | ||
| date, | ||
| timestamp, | ||
| }: CommentItemProps) => { |
There was a problem hiding this comment.
I'd make a comment: Comment prop accepting an object. No need for spreading then and you have everything contained in one object
There was a problem hiding this comment.
I'd make a
comment: Commentprop accepting an object. No need for spreading then and you have everything contained in one object
Sure, but you neeed to destructure it later or write redundant object prop calls
comment.timestamp
comment.avatar
...and so on
There was a problem hiding this comment.
I'd rather have a param destructurization than something like that tbh.
As for now the CommentItemProps is redundant as the props could be directly typed as Comment.
I used to spread props but over the time I learned in most of the cases it is better to explicitly pass props. There are some exceptions ofc (for example register function for form libs)
| const sizes = { | ||
| 24: 'w-24 h-24', | ||
| 12: 'w-12 h-12', | ||
| 10: 'w-10 h-10', | ||
| }; |
There was a problem hiding this comment.
This can be placed outside of the component (above component declaration) as a const
SIZES or AVATAR_SIZES
4 participants
Uh oh!
There was an error while loading. Please reload this page.